Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set unicode values for glyphs #3

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

cmyr
Copy link
Contributor

@cmyr cmyr commented Mar 16, 2021

I notice you ran this through some other normalization tool or something? The diff is larger than it should be. In any case this is motivated by linebender/runebender#233; preview doesn't work without unicode values set.

Feel free to ignore this PR and do it some other way if you like, I personally wouldn't like merging this if I were you 😛

@eliheuer
Copy link
Owner

Thanks! I'm not sure what is going on here, I don't remember using a normalization tool of anything.

@eliheuer eliheuer merged commit c396ba2 into eliheuer:master Mar 16, 2021
@cmyr cmyr deleted the glyph-unicode-values branch March 17, 2021 00:24
@cmyr
Copy link
Contributor Author

cmyr commented Mar 17, 2021

huh, that's weird, I have no idea what might have been going on 🤔

@xorgy
Copy link

xorgy commented Apr 4, 2021

I suggest running your glifs through xmllint. xmllint will format your files consistently, and would have reduced the size of these diffs a lot. ufoLib uses two-space indents, and I believe two-space indents are the default with xmllint as well.

@alerque
Copy link

alerque commented Apr 4, 2021

Normalizing UFO data is kind of a harry subject. cf. ufoNormalizer, pysilfont, and the many discussions that have gone on in UFO issues, the font-text working group, etc.

Also note using if I finish setting up a build workflow using fontship (see #2) normalizing the sources is a recommended (although optional) step that it will do using ufonormalizer.

If xmllint brings something to the table not already being covered I'd be interested in hearing about it, but honestly I'm skeptical a generic XML linter will have much to add.

@xorgy
Copy link

xorgy commented Apr 4, 2021

If xmllint brings something to the table not already being covered I'd be interested in hearing about it, but honestly I'm skeptical a generic XML linter will have much to add.

Well, it's not that it's "adding" anything in particular, just that these files were already indented on two spaces, so xmllint (like any good, functioning XML formatter) can reduce the noise in your patch by not messing with all of the whitespace in the whole font when you really just wanted to add a line to each glyph.

RE: ufoNormalizer: the indent thing is really the only controversy it causes. The UFO spec examples are all indented on two spaces, it's the default indent for formatted XML from libxml2, and several UFO tools in the wild prefer two spaces despite ufoNormalizer preferring tabs. I hate to get into what undoubtedly seems like a shallow argument about indent styles that is as old as time, but it really seems that ufoNormalizer is the weird one here.

I will say ufoNormalizer overall does some stuff that I don't like, like adding keys to your lib.plist, and massive strings full of timestamps all over the place. Timestamps seem like a good idea until you try to run ufoNormalizer in a filter-branch or similar operation (to clean up a non-normalized history) and find it putting different timestamps in each time.

@alerque
Copy link

alerque commented Apr 5, 2021

I think you might have misunderstood me a little. I'm not a fan of tabs in markup. The choice to use ufoNormalizer has to do with it being by far the most ubiquitously used normalizer, not because I like its indentation scheme (I don't). The argument about diffs being too large is kind of a red hearing though, as long as you use the same normalizer (or even same output library) all the time this isn't an issue. Using raw runebender output shouldn't cause churn as long as everybody uses Runebender. The thing about normalizers is you can ask (or mandate) that everyone on the project uses the same one so whatever their editor spits out can be made to produce sane diffs in the context of a project.

I've been meaning no make the normalizer used in fontship easily configurable (it already can be but you have to pass an undocument environment variable instead of setting a document project config key) so that any of the major ones could be run and would be happy for xmllint to be part of that, but my point about "not adding anything" was that I don't think it does anything extra beyond code style fixes. The UFO specific linters also do some things like cleanup invalid data, add missing keys, etc.

And yes timestamps in the file format were/are a terrrrrrrible idea. We should get that addressed upstream. I've already campaigned for them to be removed from OpenType and some other places so we can work towards reproducible builds, but there is more work to be done there.

@xorgy
Copy link

xorgy commented Apr 6, 2021

And yes timestamps in the file format were/are a terrrrrrrible idea. We should get that addressed upstream. I've already campaigned for them to be removed from OpenType and some other places so we can work towards reproducible builds, but there is more work to be done there.

Yeah, honestly I love the basic psfnormalize behaviour, and you can solve the problem of it changing the OpenType timestamps by just deleting that field (it won't add it back), but there appears to be no way to disable writing logs to files. I ran it in a git filter-branch and my best option was to remove the garbage it wrote into the working directory using rm in the filter-branch command lol.

I might just write my own, with a focus on a) adopting the most popular formatting from authoring tools and b) not touching anything or trying to be smart about embedding metadata in the font, completely reproducible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants